Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use update in tests instead of save #1149

Closed
wants to merge 5 commits into from
Closed

Conversation

anniel-stripe
Copy link
Contributor

@anniel-stripe anniel-stripe commented Nov 18, 2022

Summary

Now that save is deprecated in v8.0.0, we want to update the tests to call the static update method instead of save.

This PR preserves the functionality of most tests, except for those involving arrays because the previous array tests used legal_entity on Accounts, which no longer exists in the API, but is a protected field in account.rb so update does not work on that field. Instead, I update the items array on a Subscription object in those tests.

I also removed 2 tests involving nil id values, since that functionality was unique to save.

@remi-stripe
Copy link
Contributor

We should absolutely keep tests for .save until we remove it. This functionality is old and brittle enough that losing valuable historical tests is going to be dangerous.

c.description = "another_mn"
c.save
Stripe::Customer.construct_from(customer_fixture)
Stripe::Customer.update("cus_123", { description: "another_mn" })
end

should "updating should merge in returned properties" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure "merge in returned properties" describes what Update does. Maybe we should just delete these tests or keep them the way they are because even though it is deprecated it is good to have test coverage for .save until we entirely remove it

@anniel-stripe
Copy link
Contributor Author

Based on the conversation above, I'll close this PR and wait until we fully remove save to remove these tests. Thanks for taking a look!

@remi-stripe remi-stripe deleted the anniel-save-tests branch September 28, 2023 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants